-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[pythonic config] Add test showcasing use of Pydantic validators #12536
[pythonic config] Add test showcasing use of Pydantic validators #12536
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ |
Current dependencies on/for this PR: This comment was auto-generated by Graphite. |
0a99ef6
to
2b61f09
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add to the test plan in his PR to see how a violation of a validation error is surfaced in the config editor?
Yes, updated w/ a screenshot. Right now the error only surfaces on run start (when the config class is populated). I wonder if we get this to work as part of launchpad configuration in a stacked PR. Right now, all the validation is done vs the serialized representation of the pipeline & there's not a great way for us to serialize a user-defined validator. We could consider sending the config to the user code server on run submit, and running a check at the same time that we kick off a run, so that we can get more immediate feedback vs having to wait for the run to start? |
I think this is right behavior in the launchpad btw. We used to have server-side evaluation in the critical path in the launchpad and it caused lots of perf issues and lag. We should never try to interpret pythonic rules client-side so I actually believe the behavior you are demonstrating is the correct tradeoff. |
Summary
Introduces test coverage validating that Pydantic validators work as expected.
Test Plan
Unit test. Tested manually in launchpad - the error here shows up when the run starts. Eventually, it would be good if this validation would show up in the launchpad itself.